fix(ocap-kernel): make c-list import accounting symmetric - #1020

1. The mirror this comment promises has a hole: incrementRefCount has no kernelRefExists guard.

In the function below (:108), the object branch reads getObjectRefCount(kref) — which returns (0, 0) for a missing row — and writes it back, resurrecting a deleted object. decrementRefCount (:152) guards exactly this with !kernelRefExists(kref). This PR adds the guard at two call sites (translators.ts:88, retainForOcapURL) rather than at the primitive, so every other path still resurrects:

pinObject('ko99')   // a kref that never existed
  kernelRefExists('ko99')   === true
  getObjectRefCount('ko99') === { reachable: 1, recognizable: 1 }
  getOwner('ko99')          === undefined

Reachable with no remote involved. retireKernelObjects queues the retireImport and calls deleteKernelObject in the same breath, so there is always a window where the row is gone while an importer's c-list entry is live — the window refcount-audit.ts:111-115 exists to exempt. An increment inside it resurrects from zero, losing the surviving entry's recognizable unit, and the next setReachableFlag on that entry pushes reachable past recognizable:

F3c window:      {"gcActions":["v2 retireImport ko1"],"kernelRefExists":false,"v2StillMapped":"o-1","audit":[]}
F3c resurrected: {"reachable":1,"recognizable":1}
F3c throw:       refMismatch(set) "ko1" 2,1

That last throw is in the crank path. Local ocap-URL redemption reaches the same increment (incrementRefCount(slot, 'resolve|slot')) with only insistKRef, where the remote path fails loudly at translators.ts:88.

Verified still true at the tip. Failing in incrementRefCount for a missing object row — symmetric with the decrement's guard — closes the whole class instead of one path at a time.

Fixed. incrementRefCount now fails on a missing object row, the same way the decrement guards it. It fails instead of returning quietly, because taking a reference to something already deleted is always a bug.

I kept the two call-site guards. They refuse before an eref is allocated or a ledger row is written, and their message says what was attempted. Their comments no longer repeat the reason.

Six clist.test.ts tests were mapping krefs the kernel never had; they create the row first now. 2bf8a5c

2. This throw does not reliably fail a test.

It kills the run loop, but Kernel.ts:347 routes run-loop death into #handleRunLoopFailure, which deliberately does not rethrow, and kernel-test's makeKernel passes no onRunLoopFailure. So a violation surfaces only if that crank happens to have a pending queueMessage subscription for #failRunLoop to reject. A violation on a GC-only or reap crank, or after a test's last assertion, is logger.error'd and swallowed — and garbage-collection.test.ts uses a makeMockLogger() nobody asserts on.

Two further gaps in the coverage this PR claims:

• endowment-globals.test.ts:37 and io.test.ts:73 call Kernel.make directly rather than through makeKernel, so those kernels are not audited at all.
• The only call site is the end of a delivery crank, so refcount mutations outside one — launchVat's root pin, deleteSubcluster's release, issueOcapURL's retention, terminateAllVats — are checked only if a later crank happens to run, and never if the queue stays idle.

Worth having kernel-test pass an onRunLoopFailure that fails the test, so "a violation fails the build" is actually true.

Fixed. kernel-test passes an onRunLoopFailure now. It records the error, and afterEach/afterAll throw it, so the test fails with the message that names the kref.

I first tried rethrowing from a new turn. It does fail the run, but under endoify-node the worker exits with process.exit unexpectedly called with "-1" and the real error is never printed, so you learn nothing.

io.test.ts and endowment-globals.test.ts are audited too now. I checked the whole path by adding a second increment in pinObject: two cluster-launch tests failed with the violation, where before they passed. 3b516ab

3. The audit cannot report the one corruption it most needs to.

getObjectRefCount Fails at object.ts:111 when reachable > recognizable — which is precisely one of the drifts this module exists to diagnose. Reaching it here means the operator gets refMismatch(get) ko7 3,1 with no holder list, no expected, and none of the other violations in the same sweep. The promise branch already avoids this by reporting raw directly; the stored encoding is canonical "reachable,recognizable", so the same works for objects:

That also reports a malformed row ("NaN,0") as-is rather than throwing on it. Note it leaves the getObjectRefCount destructure at :74 unused — and since that is its only use in this file, the getObjectMethods import at :4 goes with it.

Done, took the suggestion. The getObjectRefCount destructure and the getObjectMethods import are gone with it.

Added a parameterized test for 3,1, NaN,0 and 1. All three throw if the row is read through getObjectRefCount. 6fb6332

4a. This fix has no test. Every existing assertion on 'deliver|send|target' (KernelRouter.test.ts:134) uses an object target, where item.target === target, and there is no "send to a promise that resolved to an object" delivery test at all — only splat, reject and unresolved variants. Reverting this line to target keeps the entire suite green.

The reasoning in the comment is right and I verified the accounting: item.target is what enqueueSend charged (KernelQueue.ts:446), and the routed target is held by the resolution value's resolve|slot unit instead. It just needs a case where the two differ to stay fixed.

Added a test. The item target is a promise that fulfilled to an object, so the queued and routed targets differ. It asserts the whole decrementRefCount call list, so it fails if the line goes back to target. 42cc2a2

4b. Same for the notify fix — untested on exactly the paths it exists for. KernelRouter.test.ts:552 ("promise is not in vat clist") and :585 ("no kpids to retire") assert only the return value, never that the reference was released. And the decrementRefCount(toResolve, 'deliver|notify|slot') this PR deletes was never exercised either: :516 mocks getKpidsToRetire to return [kpid], the equal case, so the toResolve !== kpid branch was dead before and after.

Both changes are correct — releasing up front covers both early returns, and nobody had charged the batch promises, since only enqueueNotify charges and only for kpid. Worth noting this decision is already load-bearing beyond this PR: the tip adds a third early return here (#resolveEndpoint returning undefined) which this ordering covers by construction. Which is all the more reason to pin it with a test.

Added. Both early returns now assert the release, and a new test retires a sibling promise in the same batch and checks that only kpid is released. 42cc2a2

5. These increments run before the checks that Fail. The state and decider checks are at :514-520, so an illegal syscall.resolve leaves a unit charged per slot with nobody holding it. This PR only removed the resolve|kpid increment from the same spot, but the audit it adds is what makes the leftover fatal rather than merely leaky — verified at the tip:

illegal resolve threw: Error: "v1" not permitted to resolve "kp1" because "its decider is v2"
ko refcount before: {"reachable":0,"recognizable":0}
ko refcount after:  {"reachable":1,"recognizable":1}
AUDIT: ko1: stored 1,1, expected 0,0 (held by: nothing)

So in audit mode a vat-driven illegal resolve reports a violation and kills the kernel. (Whether it escapes the crank rollback in practice I did not verify; the leak itself is proven.) Moving both increments below the Fail checks makes the charge conditional on the resolve being legal — I did not offer this as a suggestion because the destination lines are outside this diff.

Moved below the checks. Both illegal-resolve tests carry a slot now and assert incrementRefCount was not called. b619532

6. Please state the migration decision in the PR body.

Changing the birth baseline changes the meaning of every refcount row already on disk, and kernel-store has no schema version and no migration path (CREATE TABLE IF NOT EXISTS, no user_version). A store written by the current release opens under this code with every object still at (1, 1) and no pinnedObjects entry for vat roots. Two consequences:

// legacy store: (1,1) baseline, two flagged import entries that took no count
setObjectRefCount(kref, { reachable: 1, recognizable: 1 })
clearReachableFlag('v2', kref)   // (0,1)
clearReachableFlag('v3', kref)   // throws: "ko1" underflow -1,1

That fires inside performDropImports (gc-handlers.ts:26) on a dropImports syscall — the crank path, on an existing user's database. And since initializeAllVats uses runVat (which does not pin) and relies on the persisted pin, a legacy store's roots have none, so the last importer's drop can retire a live vat's root.

Still true at the tip — I grepped the whole stack for a store version, a refcount migration, or a refusal to open a pre-migration store and found none. Given the BREAKING marker, "a reset is required at this version" may well be the right answer; it just needs saying out loud. Worth noting alongside it that the advertised repair tool is hard to reach: recomputeRefCounts has no callers, is only obtainable by constructing a second makeKernelStore over the same database, and RefCountViolation is not re-exported from the package root.

Decision: reset required, no migration at this version. It is stated in the PR body under "Migration" and in the changelog under the BREAKING entry, with both consequences you named.

recomputeRefCounts cannot help here: it rebuilds counts, but not the root pins, so it is a diagnostic and not an upgrade path. I said that in both places, and how to reach it. RefCountViolation is exported from the package root now, as the changelog already claimed. 577c627

7a. A breaking API rename under ### Fixed is easy to miss. Two of these sub-bullets are Changed-shaped rather than Fixed-shaped: initKernelObject births at (0, 0) instead of (1, 1) (:78), and krefsToExistingErefs is renamed and now throws (:81). The rest of this file puts breaking API changes under ### Changed (cf. :50), and a consumer scanning for breakage will read that section, not this one.

Suggest dropping the rename from this entry:

and giving it its own bullet under ### Changed:

• BREAKING: krefsToExistingErefs is renamed to krefsToErefs and now throws on an unmapped kref instead of silently dropping it (#1020)

Took the suggestion. The rename is its own BREAKING bullet under ### Changed now. 577c627

7b. This overstates what the audit detects, and #1022 walks it back two PRs later: "it compares counts against the holders it finds, so a holder that should have been torn down but wasn't justifies its own count and is not detectable this way — 'a leak' overstated it." I confirmed the blind spot independently: auditRefCounts() returns [] for an object record with no holder at all, which is #1006's symptom 4. Better to state the limit correctly here than to correct it downstream:

Took the suggestion, with your wording. 577c627

7c. undoOcapURLRetention is equally public on KernelStore (it is in the exhaustive surface list at store/index.test.ts:195) but is not named here — :95 alludes to the behaviour without giving the method:

Separately, the formatting commit inserted blank lines inside the pre-existing #984 entry (:35, :39), which loosens someone else's list for no reason — worth reverting those two to keep the diff to your own entries.

Added undoOcapURLRetention. Also reverted the blank lines in the #984 entry, and the same ones in my own entry. Prettier is happy without them. 577c627

Keeping the krefs in an array like this seems algorithmically problematic. This presumes that the number of objects that a kernel will issue URLs for will be be small, which is common but not guaranteed.

Moreover, this can fail in the case of multiple URLs being issued for a given object (which is a totally normal thing to happen). While the retain/undo logic used here will work (albeit inefficiently) for the failure-of-issue case that OcapURLManager.ts anticipates, it will fail to correctly clean up in the object disavowal case since undo only removes one entry rather than all of them (and conversely, removing all of them will break things in the failure-of-issue case).

Fixed, both parts. 266eaad

The row is gone. Retentions and pins are each a count per object now — ocapURLObjects.${koid} and pinned.${koid} — so an issuance is one write whatever else is retained, rather than reading, appending to and re-sorting a row that grows without bound. I converted pinnedObjects with it: it has the same shape and the retention writes into it, so leaving it would only have moved the cost one layer down. getPinnedObjects now names each object once and getPinCount gives the number of pins, which is what the audit needs to credit a unit per pin.

Multiple URLs for one object. Counting is what makes that work where a multiset didn't: the second issuance takes the count to 2, and the pin is taken once on 0→1 and spent once on 1→0. A failed mint decrements, so it can no longer take away the retention a live URL depends on — which was the only reason the entry was per issuance.

Disavowal is its own operation. You're right that undo cannot serve both, so it no longer tries: releaseOcapURLRetentions(kref) drops the count and the pin together, and undoOcapURLRetention keeps meaning "one issuance whose URL was never minted".

Nothing calls the release yet — I stopped short of wiring revoke to it here. Revocation only sets its flag today, so a revoked object stays retained and its URLs redeem to a kref that rejects deliveries with OBJECT_REVOKED. Dropping the retention there makes the target collectable, and redeeming an outstanding URL for it then reaches translateRefKtoE, which this PR made throw on a deleted kref — in #handleRedeemURLRequest, outside the try that turns a redemption failure into a reply. So reclaiming on revoke wants a clean "that capability is gone" answer first, which is its own change. Raised as a follow-up; say the word and I'll land it here instead.

Pinned by tests: the counting and the release in store/index.test.ts, pin counts in pinned.test.ts (over a real map now, rather than asserting on kv.set arguments), and the per-object semantics through the manager in OcapURLManager.test.ts. Root test:dev:quiet 53/53, build 31/31, lint clean.